Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core,discovery: add checks for PriceInfo.pixelsPerUnit==0 #1813

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

kyriediculous
Copy link
Contributor

@kyriediculous kyriediculous commented Mar 25, 2021

What does this pull request do? Explain your changes. (required)
Using common.RatPrinceInfo for converting net.PrinceInfo -> big.Rat and handling the error case.

This function already does a check for the PriceInfo denominator being 0

Specific updates (required)

  • Using common.RatPriceInfo
  • Add unit tests

Didn't explicitly test the predicate function, would require us to move it to global scope I believe. Can always do this still.

How did you test each of these updates (required)

Does this pull request close any open issues?

Checklist:

@kyriediculous kyriediculous requested a review from yondonfu March 25, 2021 21:02
@kyriediculous kyriediculous marked this pull request as ready for review March 25, 2021 21:03
CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
discovery/db_discovery.go Show resolved Hide resolved
@kyriediculous kyriediculous requested a review from yondonfu March 25, 2021 22:21
discovery/db_discovery.go Outdated Show resolved Hide resolved
@kyriediculous
Copy link
Contributor Author

kyriediculous commented Mar 25, 2021 via email

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squashing!

@kyriediculous kyriediculous merged commit 9a4f56a into master Mar 25, 2021
@kyriediculous kyriediculous deleted the nv/nil-ppu branch March 25, 2021 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants